feat(security): add markdown-aware skill security scanner (Phase 1) by diberry · Pull Request #882 · github/squad

diberry · 2026-04-06T15:25:24Z

Summary

Extends security-review.mjs with Tier 1 pattern detection for skill markdown files (.copilot/skills/**/*.md and .squad/skills/**/*.md). Zero new dependencies, zero workflow changes, ~2-3s added scan time.

What's new

30 security patterns across 4 categories:

P1: Embedded Credentials (10 patterns) — AWS keys, GitHub PAT/OAuth/App tokens, OpenAI keys, private keys, JWT, npm tokens, Slack tokens, generic secret assignments
P2: Credential File Reads (7 patterns) — cat/type/readFileSync/Get-Content for .env, .ssh/id_rsa, .aws/credentials, .npmrc, .netrc, .pem/.key files (excludes .env.example/.env.sample/.env.template)
P3: Download-and-Execute (8 patterns) — curl|bash, wget|bash, irm|iex, Invoke-Expression, powershell -enc, eval/source/bash process substitution
P4: Privilege Escalation (5 patterns) — sudo bash/sh/rm, RunAs admin, Set-ExecutionPolicy Bypass/Unrestricted, chmod 777

Phase 1 Suppression (5 layers):

Fenced code blocks (CommonMark-compliant: backtick/tilde, variable length, leading spaces)
Inline code spans (backtick pairs) — prevents false positives on documentation examples
Regex-doc table rows — suppresses credential matches on tables documenting regex patterns
Placeholder tokens (sk-..., ghp_xxxx, AKIA..., angle brackets)
Fail-safe: unclosed fences = unsuppressed (fail-open for security)

Architecture:

Pure scanSkillContent(content, filePath) function exported for testing
Main flow wrapped in run() with import.meta.url guard (importing for tests doesn't trigger git/console side effects)
Existing 8 checks untouched; .copilot/skills/ skip for unsafe-git (check Feature: Issue Assignment & Auto-Triage for Squad Members #4) preserved
New categories: skill-credentials, skill-credential-file-read, skill-download-exec, skill-privilege-escalation

Testing

110 new tests + 37 existing tests = 147 tests, all passing:

Every pattern has positive + negative cases
Fenced code block, inline code, regex-doc table, placeholder, and fail-open suppression tested
Golden corpus: zero false positives across all 35 existing skill files

Not in scope (Phase 2)

Context classifier (anti-pattern sections, heading-aware suppression)
P5-P11 patterns
Credentialed URL detection, frontmatter injection

Closes #881, supersedes #879

Working as Booster (CI/CD Engineer)

github-actions · 2026-04-06T15:25:37Z

🟡 Impact Analysis — PR #882

Risk tier: 🟡 MEDIUM

📊 Summary

Metric	Count
Files changed	4
Files added	2
Files modified	2
Files deleted	0
Modules touched	3

🎯 Risk Factors

4 files changed (≤5 → LOW)
3 modules touched (2-4 → MEDIUM)

📦 Modules Affected

root (1 file)

.changeset/skill-security-scanner.md

scripts (1 file)

scripts/security-review.mjs

tests (2 files)

test/scripts/security-review-skills.test.ts
test/scripts/security-review.test.ts

This report is generated automatically for every PR. See #733 for details.

Copilot

Pull request overview

Adds Phase 1 markdown-aware security scanning for skill files to the existing scripts/security-review.mjs repo-health scanner, with accompanying test coverage and a changeset.

Changes:

Extend security-review.mjs to scan changed skill markdown files under .copilot/skills/** and .squad/skills/** for Tier 1 patterns (credentials, download-and-exec, privilege escalation).
Export a pure scanSkillContent() function and guard script execution so it can be imported safely from tests.
Add a dedicated Vitest suite for skill scanning behavior and register the new finding categories in the existing schema test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`scripts/security-review.mjs`	Adds markdown-aware scanning (`scanSkillContent`) and integrates it into the main security-review run.
`test/scripts/security-review.test.ts`	Extends the allowed category allowlist to include new `skill-*` categories.
`test/scripts/security-review-skills.test.ts`	New comprehensive unit tests for skill scanning patterns + suppression behavior + golden corpus.
`.changeset/skill-security-scanner.md`	Declares a patch release for the CLI package to ship the new scanner behavior.

github-actions · 2026-04-06T15:32:27Z

🛫 PR Readiness Check

ℹ️ This comment updates on each push. Last checked: commit 4076c6d

PR Scope: 🔧 Infrastructure

⚠️ 3 item(s) to address before review

Status	Check	Details
✅	Single commit	1 commit — clean history
✅	Not in draft	Ready for review
✅	Branch up to date	Up to date with dev
❌	Copilot review	No Copilot review yet — it may still be processing
✅	Changeset present	Changeset file found
✅	Scope clean	No .squad/ or docs/proposals/ files
✅	No merge conflicts	No merge conflicts
❌	Copilot threads resolved	2 unresolved Copilot thread(s) — fix and resolve before merging
❌	CI passing	1 check(s) still running
✅	Issue linked	Issue reference found
✅	Protected files	No protected bootstrap files changed

Files Changed (4 files, +866 −0)

File	+/−
`.changeset/skill-security-scanner.md`	+5 −0
`scripts/security-review.mjs`	+206 −0
`test/scripts/security-review-skills.test.ts`	+651 −0
`test/scripts/security-review.test.ts`	+4 −0

Total: +866 −0

This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.

Extends security-review.mjs with Tier 1 pattern detection for skill markdown files (.copilot/skills/**/*.md and .squad/skills/**/*.md): - P1: 10 embedded credential patterns (AWS, GitHub, OpenAI, JWT, etc.) - P2: 7 credential file read patterns (.env, .ssh/id_rsa, .aws/credentials, .npmrc, .netrc) - P3: 8 download-and-execute patterns (curl|bash, irm|iex, etc.) - P4: 5 privilege escalation patterns (sudo, RunAs, chmod 777, etc.) Suppression (Phase 1): - Fenced code blocks (CommonMark-compliant: backtick/tilde, variable length) - Inline code spans (backtick pairs) - Regex-doc table rows (suppress credential matches in pattern documentation) - Placeholder tokens (sk-..., ghp_xxxx, AKIA..., angle brackets) - Fail-safe: unclosed fences = unsuppressed (fail-open for security) Architecture: - Pure scanSkillContent() function exported for unit testing - Main flow wrapped in run() with import.meta.url guard - Existing checks unchanged; .copilot/skills skip for unsafe-git preserved Testing: 110 new tests covering all patterns, suppression, and golden corpus (zero false positives across 35 existing skill files). Closes #881, supersedes #879 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

diberry · 2026-04-06T15:52:22Z

How This Works — Quick Guide for Reviewers

What changed: scripts/security-review.mjs now scans skill files (.copilot/skills/**/*.md and .squad/skills/**/*.md) for security patterns when they appear in a PR diff.

How it runs: Same VM, same job, same step as the existing security review in squad-repo-health.yml. No new runners, no new workflows. Adds ~3 seconds to the existing step.

What it catches (Phase 1 — high-confidence only):

P1: Embedded credentials — real AWS keys, GitHub PATs, OpenAI keys, PEM blocks, JWTs, npm/Slack tokens
P2: Credential file reads — instructions to cat .env, read ~/.ssh/id_rsa, etc.
P3: Download-and-execute — curl | bash, irm | iex, powershell -enc, etc.
P4: Privilege escalation — sudo bash, chmod 777, Set-ExecutionPolicy Bypass, etc.

What it does NOT catch (deferred to Phase 2):

Unsafe git operations (git add ., --force push) — existing skip preserved because the diff parser can't distinguish "do X" from "never do X" in documentation
Generic paths/IPs/URLs — too noisy for skill docs that legitimately reference localhost, C:\, etc.

Suppression (avoids false positives):

Lines inside fenced code blocks (CommonMark-compliant, handles 4-backtick nesting)
Inline code spans (backtick-wrapped)
Regex documentation tables (pattern docs in secret-handling skill)
Placeholder tokens (sk-..., ghp_xxxx, AKIA...)
Fail-safe: unclosed fences = unsuppressed (fail-open for security)

Enforcement level: Informational only. Posts a PR comment if findings exist. Does NOT block merge or fail CI. This is a heads-up, not a gate — appropriate for Phase 1 where suppression is simple.

Test coverage: 110 new tests + golden corpus validation (zero false positives across all 35 existing skill files). Pure scanSkillContent() export enables direct unit testing without git side effects.

Cost: Zero additional Actions minutes. Zero new dependencies. ~3 seconds added to existing runner.

PRD: #881 | Supersedes: #879 | Inspired by: microsoft/waza ComplianceChecker patterns

tamirdresher

✅ Reviewed by Tamir + Squad.

Excellent security scanner — clean architecture with pure scanSkillContent() for testability. 30 patterns across 4 categories with 5-layer suppression is thorough. The fail-open on unclosed fences is the right security posture. 110 new tests + golden corpus validation (zero false positives on 35 existing skills) gives strong confidence.

Two minor Copilot notes (non-blocking):

Unused statSync import in test file
.squad/skills skip for unsafe-git already handled by the existing skip line

Phase 1 informational-only is the right approach. Ship it.

…882) Extends security-review.mjs with Tier 1 pattern detection for skill markdown files (.copilot/skills/**/*.md and .squad/skills/**/*.md): - P1: 10 embedded credential patterns (AWS, GitHub, OpenAI, JWT, etc.) - P2: 7 credential file read patterns (.env, .ssh/id_rsa, .aws/credentials, .npmrc, .netrc) - P3: 8 download-and-execute patterns (curl|bash, irm|iex, etc.) - P4: 5 privilege escalation patterns (sudo, RunAs, chmod 777, etc.) Suppression (Phase 1): - Fenced code blocks (CommonMark-compliant: backtick/tilde, variable length) - Inline code spans (backtick pairs) - Regex-doc table rows (suppress credential matches in pattern documentation) - Placeholder tokens (sk-..., ghp_xxxx, AKIA..., angle brackets) - Fail-safe: unclosed fences = unsuppressed (fail-open for security) Architecture: - Pure scanSkillContent() function exported for unit testing - Main flow wrapped in run() with import.meta.url guard - Existing checks unchanged; .copilot/skills skip for unsafe-git preserved Testing: 110 new tests covering all patterns, suppression, and golden corpus (zero false positives across 35 existing skill files). Closes #881, supersedes #879 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…tion security-review-skills.test.ts was added to main by PR #882 but references scanSkillContent which was subsequently removed. This test fails on main too. Removing from promotion branch since it's not part of insider. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

diberry added the enhancement New feature or improvement label Apr 6, 2026

Copilot AI review requested due to automatic review settings April 6, 2026 15:25

diberry added the enhancement New feature or improvement label Apr 6, 2026

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Comment thread scripts/security-review.mjs Resolved

Comment thread test/scripts/security-review-skills.test.ts Resolved

diberry force-pushed the squad/881-skill-security-scanner branch from 14d1572 to 4076c6d Compare April 6, 2026 15:31

diberry force-pushed the squad/881-skill-security-scanner branch from 4076c6d to af4993d Compare April 6, 2026 15:44

tamirdresher approved these changes Apr 6, 2026

View reviewed changes

diberry merged commit 7479206 into dev Apr 6, 2026
11 checks passed

diberry mentioned this pull request Apr 6, 2026

feat: Adopt waza ComplianceChecker for skill security scanning #879

Closed

6 tasks

Navigation Menu

Repository navigation

feat(security): add markdown-aware skill security scanner (Phase 1) - #882

feat(security): add markdown-aware skill security scanner (Phase 1)#882
diberry merged 1 commit into
devfrom
squad/881-skill-security-scanner

diberry commented Apr 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2026

Uh oh!

diberry commented Apr 6, 2026

Uh oh!

tamirdresher left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

diberry commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's new

Testing

Not in scope (Phase 2)

Uh oh!

github-actions Bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟡 Impact Analysis — PR #882

📊 Summary

🎯 Risk Factors

📦 Modules Affected

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 6, 2026

🛫 PR Readiness Check

⚠️ 3 item(s) to address before review

Files Changed (4 files, +866 −0)

Uh oh!

diberry commented Apr 6, 2026

How This Works — Quick Guide for Reviewers

Uh oh!

tamirdresher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diberry commented Apr 6, 2026 •

edited

Loading

github-actions Bot commented Apr 6, 2026 •

edited

Loading